Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

adding slack notifier #9076 #9350

Merged
merged 2 commits into from
Sep 26, 2019
Merged

adding slack notifier #9076 #9350

merged 2 commits into from
Sep 26, 2019

Conversation

bmcgough
Copy link
Contributor

Adding slack webhook notifier - clone of pushbullet mostly

Motivation and Context

Allow ZED notification via slack incoming webhook

#9076

Description

Copied and updated pushbullet function to change message format and url for slack.

How Has This Been Tested?

I've installed on vanilla system with ZED_SLACK_WEBHOOK_URL set, and verified messaging was successful.

ZFS 0.8.0 on Ubuntu 18.04

I updated comments in associated files, but do not find any documentation about pushbullet, so did not update.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Sep 23, 2019
@codecov
Copy link

codecov bot commented Sep 24, 2019 •
edited
Loading

Codecov Report

Merging #9350 into master will decrease coverage by 17.67%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #9350       +/-   ##
===========================================
- Coverage   79.06%   61.38%   -17.68%     
===========================================
  Files         401      352       -49     
  Lines      122495   116276     -6219     
===========================================
- Hits        96846    71378    -25468     
- Misses      25649    44898    +19249
Flag Coverage Δ
#kernel 61.89% <ø> (-17.87%) ⬇️
#user 55.85% <ø> (-10.79%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73d7820...256edd6. Read the comment docs.

behlendorf
behlendorf approved these changes Sep 24, 2019
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to manually test this myself, but after reading through the slack documentation and PR it looks correct. And since it's disabled by default this is very low risk. It would be nice to include in the zed(8) man page a brief explanation on how to configure the available notifications (email, pushbullet, slack). I don't think that needs to block this PR, but it would be wonderful is someone tackled writing this up.

@behlendorf
Copy link
Contributor

@bmcgough would you please add your "Signed-off-by" to the commit when addressing the small nit. See https://github.com/zfsonlinux/zfs/blob/master/.github/CONTRIBUTING.md#signed-off-by for details.

richardelling
richardelling reviewed Sep 24, 2019
View reviewed changes
Copy link
Contributor

@richardelling richardelling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few nits, otherwise this will be very useful, thanks!

@bmcgough
Copy link
Contributor Author

Signed-off-by: Ben McGough [email protected]

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 25, 2019
@behlendorf
Copy link
Contributor

@richardelling @freqlabs if you're OK with the code as-is would you mind approving this PR.

@behlendorf behlendorf merged commit 3768db2 into openzfs:master Sep 26, 2019
@behlendorf
Copy link
Contributor

@bmcgough merged, thanks for taking the time to contribute this additional notifier!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf approved these changes

@richardelling richardelling richardelling approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bmcgough @behlendorf @richardelling